Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrading to GraalVM 21.0.2 #8883

Merged
merged 4 commits into from
Feb 19, 2024
Merged

Upgrading to GraalVM 21.0.2 #8883

merged 4 commits into from
Feb 19, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 29, 2024

Pull Request Description

Upgrades to latest GraalVM 21.0.2

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code has been tested:
    • Unit tests continue to pass

@Akirathan
Copy link
Member

A License check is failing. I know how to fix that. Let me push some commits...

@Akirathan
Copy link
Member

Akirathan commented Jan 29, 2024

I am struggling to see what is wrong with the license configuration. When I run enso/gatherLicenses locally, it generates a different number of warnings than on the CI. How can I reproduce https://github.com/enso-org/enso/actions/runs/7694191923/job/20964475421?pr=8883#step:10:1471 locally?

Please help @radeusgd

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

@radeusgd
Copy link
Member

I am struggling to see what is wrong with the license configuration. When I run enso/gatherLicenses locally, it generates a different number of warnings than on the CI. How can I reproduce enso-org/enso/actions/runs/7694191923/job/20964475421?pr=8883#step:10:1471 locally?

Please help @radeusgd

GitHub**Upgrading to GraalVM 21.0.2 · enso-org/enso@41b7efc**Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

CI does not run gatherLicenses at all. It only runs verifyLicensePackages which just checks if the hashcode of the dependency config and the reports are the same as the one written in the committed report-state file. The warnings count is also written to that file - so that the CI knows if it should be failing - but the count of warnings is generated only locally and has to be committed. Maybe the differing amount of warnings is from a previous run of the tool by Jaroslav? Anyway, you should just try to update the config locally to have 0 warnings and then you can commit the 'fixed' report state and you should be able to get this to pass.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jan 30, 2024

Anyway, you should just try to update the config locally to have 0 warnings

How does one "update the config"? Is there a command to invoke to generate the update? I am dreaming about sbt gatherLicenses to update all the license file on disk. Then I just review the changes and integrated them.

Instead of that I get a page like this:
obrazek

What that is supposed to mean? Yes, I found licenses.md...

Found legal review configuration for package org.graalvm.polyglot.polyglot-23.1.0, but no such dependency has been found. Perhaps it has been removed?

Eh? Where did the tool find it? Why can't I even grep for it? Can't it have a link?

(NotReviewed)

A red message. Oh, surprise, it is clickable! How can one find that out without accidentally clicking on it!?

What's the point of clicking each NotReviewed and selecting ignore? Making the changes automatically would have the same effect and would be way less work while providing the same legal shielding (e.g. almost none at all).

@radeusgd
Copy link
Member

radeusgd commented Jan 30, 2024

Found legal review configuration for package org.graalvm.polyglot.polyglot-23.1.0, but no such dependency has been found. Perhaps it has been removed?

Eh? Where did the tool find it? Why can't I even grep for it? Can't it have a link?

In the subdirectory for a given component within tools/legal-review, as stated by the documentation you linked.

A red message. Oh, surprise, it is clickable! How can one find that out without accidentally clicking on it!?

Sorry that the tool design is lacking, it was made in a very short time and we never invested more time in improving it. I made it mostly as a replacement for manually gathering licenses of about 100 of our dependencies back in the day. I'm happy to improve it if we find time for this.

What's the point of clicking each NotReviewed and selecting ignore?

The idea was to rather select 'Keep with context' most of the time. Licenses like Apache specify that we should redistribute copyright notices that are found within the libraries. This was the simplest way to ensure all copyrights are redistributed (even if maybe it would not be strictly necessary to keep all of them).

But as you say, it's a very rudimentary system. Until we have a proper legal team review this, I think it's a decent 'due diligence' we can be doing in trying to satisfy the copyright notice requirements.

But I definitely agree it is very minimal (it was a 'better anything than nothing' solution) and we should actually get our official distribution reviewed by someone competent (a lawyer) to ensure all licenses we are using are indeed compatible and we satisfy the requirements in the distribution.

The idea was that 'Ignore' is only used for false-positives i.e. stuff that matches that is not a copyright notice.


If you think we could use something else instead of this tool, I have nothing against migrating.

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Feb 19, 2024
@mergify mergify bot merged commit f1d4e54 into develop Feb 19, 2024
26 of 29 checks passed
@mergify mergify bot deleted the wip/jtulach/GraalVM_21_0_2 branch February 19, 2024 12:09
@JaroslavTulach
Copy link
Member Author

Thanks a lot for finishing and merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants